Repository navigation
Conversation
Wrap controls only at widths at or below 760px and anchor Style/Export menus to the toolbar. Add a real-Chrome regression for English/Chinese, four styles and narrow/desktop viewports; retain keyboard access and print hiding. Rebuild the deterministic Skill ZIP with official Node 22.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks @BlueboxC for turning the shared #1068 evidence into this focused P0 fix and keeping the existing ownership split. I reviewed the 12af763 CSS change: it is limited to <=760px toolbar wrapping and menu anchoring, with desktop rules and print hiding preserved. I did not find a blocking regression in that CSS scope. The new toolbar browser case actually passed in CI 40092367553; the remaining full-regression failure is the centrally tracked README showcase drift, not a newly observed toolbar failure. Two small test-evidence clarifications, without asking you to repeat the published matrix: the Escape helper focuses the toggle before sending Escape, while the handler is on the panel, and there is no closing/focus-return assertion afterward. Please either make that focused assertion exercise the panel correctly or avoid claiming Escape coverage. Also, assigning dataset.style covers the style CSS but does not switch the selected button label, so it does not by itself prove every real selected-label width fits. Please personally review the narrow-screen wrapping and the intentional menu position below the whole toolbar, then complete the author-review checkbox only once that review is actually done. Keep the PR draft pending that confirmation and the remaining acceptance/merge decision. This comment is not a merge approval; no broad additional capture campaign is requested. |
|
Addressed the two test-evidence gaps in 3ff1f02. The existing matrix now chooses each style through its real menu item and verifies its selected label before measuring containment. Escape is sent from the item focused by opening the menu; the test checks hidden state, Adding the closure assertion to the old test reproduced the problem: the menu stayed open because the helper had moved focus to the toggle. The corrected 24 locale/style/viewport combinations pass, and the full nine-case Explainer Chrome suite passes on official Node22.23.2. No new screenshots or expanded capture campaign, and no runtime/CSS/package change in this follow-up. The PR remains draft with the personal author-review checkbox unchecked. Your requested human review and upstream acceptance are still separate. The previously disclosed README showcase drift has not been hidden. BlueboxC |
|
Thanks for the focused follow-up at 3ff1f02. I verified that the test now selects styles through the real menu and checks the selected label, and sends Escape with a menu item focused before asserting hidden state, aria-expanded=false and focus returning to the toggle. Both earlier test-evidence findings are closed. I also checked this head's CI 40095300022: the narrow-viewport toolbar case actually passes, with browser shards 135 + 170 passing and no skips. The remaining full-regression failure is the separate shared README showcase drift; I found no new toolbar failure. No broader matrix or runtime changes are requested. Thank you for keeping the draft and personal-review confirmation accurate; those remaining acceptance steps and merge approval are still separate. |
tt-a1i
left a comment
There was a problem hiding this comment.
Agent-assisted review; this pass was read-only and reused the explicitly identified prior evidence.
The focused CSS P0 remains worth landing. At unchanged 3ff1f02, the selected-label and Escape/focus-return findings are already addressed; I am not asking for another published matrix. Current dev 10b4dd5 still has the same Explainer reader CSS/JS that produced the original overflow, so the new baseline has not superseded this fix.
Please refresh this existing branch against current dev, retaining the Apache/historical-MIT notices and the newer native-Viewer close-event test synchronization. Rebuild the archive from the combined source rather than selecting the old ZIP. The existing toolbar case and affected Explainer file are sufficient focused checks after that update, alongside required final-head CI. The old README drift is historical evidence, not a reason to waive new-head checks.
The requested personal review of narrow wrapping and menu anchoring is still marked pending in the body. Keep that status accurate until the review is actually completed. No P1/P2 work or broader capture campaign is requested.
Problem and value
Agreed CSS P0 from #1068. The English Explainer toolbar overflows a 320px viewport and obstructs Style/Export.
Wrap the toolbar at <=760px and anchor menus to the wrapping container. The runtime delta against inspected dev
10b4dd5c5f2264baa315ee65a9b4cc23c13ea5f0remains one media rule. No JavaScript, dependency, release-identity or P1/P2 feature change.AI assistance and accountability
This remains a draft. Personal human-review confirmation is pending.
Stability impact
Shared Explainer reader CSS; desktop >760px and existing print behavior remain preserved. The existing browser fixture checks observable containment, Style/Export operation, Escape/focus restoration and hidden print controls. It does not freeze layout coordinates or introduce a parallel test framework.
Current verification
Head
cd50794merges the inspected dev revision above into this branch. Compared with that dev tree, only the CSS rule, its existing-file regression and canonical ZIP differ.npm run test:generatedpasses.Visual evidence and generated artifacts
The browser regression is reproducible from this head and compares the same document, locale, style and viewport. Perceptual human review is pending. Gallery pages do not embed the Explainer reader change, so no unrelated Gallery refresh is included. No release, deployment or repository-generation benchmark is claimed.
BlueboxC